[Breaking change] Add fine field option #991
Conversation
@Yurlungur: Well, I have only tested for unigrid so not totally sure it works for prolongation and restriction. But at least it runs with AMR w/o segfaults. |
I suppose we should check for correctness with AMR lol. |
Co-authored-by: Adam M. Dempsey <adamdemps@gmail.com>
| if (cl == CellLevel::same) { | ||
| if (el == TE::CC) { | ||
| return cell_volume_; | ||
| } else if (el == TE::F1) { | ||
| return area_[X1DIR - 1]; | ||
| } else if (el == TE::F2) { | ||
| return area_[X2DIR - 1]; | ||
| } else if (el == TE::F3) { | ||
| return area_[X3DIR - 1]; | ||
| } else if (el == TE::E1) { | ||
| return dx_[X1DIR - 1]; | ||
| } else if (el == TE::E2) { | ||
| return dx_[X2DIR - 1]; | ||
| } else if (el == TE::E3) { | ||
| return dx_[X3DIR - 1]; | ||
| } else if (el == TE::NN) { | ||
| return 1.0; | ||
| } |
There was a problem hiding this comment.
Is this all new or can we call the older volume method which defaults to CellLevel::same?
There was a problem hiding this comment.
the old Volume method is templated on the topological element. Here I added a new Volume overload that is not templated and takes a topological type and a cell level as an argument. It should give the same results as the templated volume function.
There was a problem hiding this comment.
What I meant was that for the CellLevel::same branch can you call the old method?
There was a problem hiding this comment.
You could, but since the old one is templated on TE and this one is not it doesn't save any lines of code since you would have to call each template by hand. I am not actually sure that this really needs to be templates though.
There was a problem hiding this comment.
Oh I see. Could the old one optionally take a runtime? That might be useful...
There was a problem hiding this comment.
Yeah, I think the best solution might be to have the old one just pass the template argument as a regular argument to the new one and have the new one have a default argument of cell level same. I doubt that has any performance impact.
| auto start_send = tl.AddTask(none, parthenon::StartReceiveBoundaryBuffers, mc1); | ||
| auto start_flxcor = tl.AddTask(none, parthenon::StartReceiveFluxCorrections, mc0); |
There was a problem hiding this comment.
Independent of this PR but relevant in the context of recent discussion around exhausting MPI limits, it might be worth to change the pattern here to first start all receives (in their own TaskRegion) rather than starting them "per pack" as the behavior will then depend on the pack size.
There was a problem hiding this comment.
Yeah, that seems reasonable.
| pmb->par_for_bndry( | ||
| PARTHENON_AUTO_LABEL, nb, domain, el, coarse, fine, | ||
| KOKKOS_LAMBDA(const int &l, const int &k, const int &j, const int &i) { | ||
| if (TYPE == BCType::Reflect) { | ||
| const bool reflect = (q(b, el, l).vector_component == DIR); | ||
| q(b, el, l, k, j, i) = | ||
| (reflect ? -1.0 : 1.0) * q(b, el, l, X3 ? offset - k : k, | ||
| X2 ? offset - j : j, X1 ? offset - i : i); | ||
| } else if (TYPE == BCType::FixedFace) { | ||
| q(b, el, l, k, j, i) = | ||
| 2.0 * val - q(b, el, l, X3 ? offset - k : k, X2 ? offset - j : j, | ||
| X1 ? offset - i : i); | ||
| } else if (TYPE == BCType::ConstantDeriv) { | ||
| Real dq = q(b, el, l, X3 ? ref + offsetin : k, X2 ? ref + offsetin : j, | ||
| X1 ? ref + offsetin : i) - | ||
| q(b, el, l, X3 ? ref - offsetout : k, X2 ? ref - offsetout : j, | ||
| X1 ? ref - offsetout : i); | ||
| Real delta = 0.0; |
There was a problem hiding this comment.
Given the complexity of this code (and above in this file) is it worth to update the new test case for use of non-periodic boundary conditions?
There was a problem hiding this comment.
yes, I think we want test coverage for all types of boundary conditions, different forest configurations, sparse and non-sparse, and regular and fine fields. Once the rest of these PRs that rely on this one are in, I was going to make another PR adding regression tests, add gold files, and maybe remove the old sparse advection test.
| } | ||
| } else if (cl == CellLevel::fine) { | ||
| if (el == TE::CC) { | ||
| return cell_volume_ / 8.0; |
There was a problem hiding this comment.
Is this generally true (i.e., design decision) for <3D sims?
I don't recall how we handle this right now.
There was a problem hiding this comment.
I think that how we deal with, e.g., 2D simulations is that they are actually 3D (so the volume of a cell is a 3-volume) but we just assume they have translational symmetry in the z-direction. In that case, I think dividing by 8 here for all ndim is the right thing to do.
| # Athena++ astrophysical MHD code | ||
| # Copyright(C) 2014 James M. Stone <jmstone@princeton.edu> and other code contributors | ||
| # Licensed under the 3-clause BSD License, see LICENSE file for details |
|
This broke the existing use of If we are not using fine fields, should the |
BenWibking
changed the title
|
@BenWibking: Yes, the |
PR Summary
This PR adds the new flag
Metadata::Fine, which indicates that the field should have twice the resolution of default fields. Fields with this flag can exchange ghosts, and be prolongated and restricted similarly to normal fields. They can also be output, although none of the plotting scripts have been updated to deal with fine fields. This feature is in use in Assail.PR Checklist